Fix black captures from servers announcing DesktopSize before pixel data (#90) - #343
Conversation
A framebuffer update whose only rectangle is the PSEUDO_DESKTOP_SIZE pseudo-encoding used to complete a pending refresh: updateDesktopSize replaces the screen with a black canvas and commitUpdate fired the deferred regardless, so captureScreen saved the black canvas and the CLI exited before the server's first pixel frame arrived. TightVNC on Windows answers the first non-incremental update request exactly this way, which is the black-screenshot report in #90 (2017-2025). The DesktopSize rectangle is now excluded from the update's content rectangles, matching the existing QEMU key-event pseudo-rect handling, and a pending refresh whose update carried no content re-requests the framebuffer instead of completing. Fixes #90. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
||
| def commitUpdate(self, rectangles: list[rfb.Rect] | None = None) -> None: | ||
| if self.deferred: | ||
| if not rectangles: |
There was a problem hiding this comment.
should commitUpdate even get called if we are properly deleting the pseudo rectangle
There was a problem hiding this comment.
🤖 Claude Code: Still needs to be called — it's the extension point, not incidental plumbing. Proof: the base class already routes a genuinely-empty update (server sends rect count 0) through the same commitUpdate(self.rectanglePos) call in _doConnection, so "zero rectangles delivered" already meant "call commitUpdate anyway" before this fix existed. Moving the retry policy down into rfb.py instead would only patch the DesktopSize case and leave the QEMU-pseudo-rect-only case and a literal empty update still completing on nothing.
What is fixable: the append-then-delete on rectanglePos for the two pseudo-encodings is backwards — skip the append instead of adding and undoing it. Saving that as a follow-up rather than folding it into this PR.
Issue references belong in the commit message and CHANGELOG, not inline comments that outlive the issue.
tests/unit/test_issue_90.py landed on main via #342 after this branch was cut; left in place it still carries @unittest.expectedFailure, so once this fix lands the case flips to an unexpected success and `make test` fails. Delete it per the triage-artifact convention now that its behaviour is covered by the two tests this PR added to test_client.py. Also resolves the CHANGELOG.rst UNRELEASED-section conflict and adds the missing @sibson attribution to the #90 entry.
|
🤖 Claude Code: Verified and updated for merge. Fix logic — Regression coverage — the two new tests in Staleness fix — the branch was cut before #342 merged, so it didn't touch the triage artifact #342 added, Verification, from a worktree off the updated branch:
Not merging or approving — leaving that to @sibson. |
Both comments asserted what TightVNC does with no citation -- motivation for the fix, not a fact the code needs. The repo's own convention already strips issue/PR references from code comments, so there's nowhere to cite it from; drop the claim and keep the parts grounded in this file: what a pixel-free update carries, and why completing the refresh on one would hand back a blank canvas.
|
🤖 Claude Code: Ran a comment-quality sweep (writing-code-comments skill) over the diff and pushed a follow-up (079a920). Two comments asserted "TightVNC answers the first update request this way" with no citation — real motivation for the fix, but unsourced in place, and the repo's own convention already strips issue/PR refs from code comments so there's nowhere to cite it from. Trimmed both down to the parts grounded in this file: what a pixel-free update carries, and why completing the refresh on one hands back a blank canvas. The Re-verified after the change: |
| if self.deferred: | ||
| if not rectangles: | ||
| # A pixel-free update, e.g. only a DesktopSize pseudo-rectangle. | ||
| # Completing the refresh now would hand over the bare black |
There was a problem hiding this comment.
🤖 Claude Code: Trimmed further (838350d) — dropped the DesktopSize example and the "black canvas" framing, which only held on first connect (updateDesktopSize pastes the prior screen back in on a resize, so it is not always black). Now: "No rectangle in this update painted self.screen; wait for one that does before completing the refresh."
Drop the DesktopSize example and the "black canvas" framing -- the latter only holds on first connect (updateDesktopSize pastes prior content back in on a resize), so it overclaimed for the general case. State the actual guard instead: nothing in this update painted the screen.
2 participants
Fixes #90 — black screenshots against TightVNC on Windows, reported 2017–2025.
Mechanism (full analysis in the issue comment): TightVNC answers the first non-incremental
FramebufferUpdateRequestwith an update containing only the DesktopSize pseudo-rectangle.updateDesktopSizereplacesself.screenwith a black canvas,_doConnectionfiredcommitUpdateunconditionally once the rectangle count hit zero, the pending capture completed, and the CLI saved the black canvas and exited — before the server's pixel frame (sent moments later) arrived.The fix, two small parts:
rfb.py: the DesktopSize rect is removed fromrectanglePosbefore decoding, exactly as the QEMU key-event pseudo-rect already is ("undo append as this is no real update") — pseudo-rectangles carrying no pixels no longer count as update content.client.py:commitUpdatewith a pending deferred and no content rectangles re-issues a non-incrementalframebufferUpdateRequestand keeps waiting, instead of completing the refresh with nothing on the canvas.A server that answered every non-incremental request with a content-free update could keep the client requesting until its timeout; real servers send content, and the per-client
timeoutstill bounds the wait either way.Regression tests live in their permanent home (
tests/unit/test_client.py): a desktop-size-only update must not complete a refresh (and must re-request), and the refresh completes normally once pixel data arrives. Full unit suite green, flake8 clean.End-to-end validation: the scripted server replaying TightVNC's logged behaviour (full repro kit in the issue thread) flips from a black capture (
extrema ((0,0),(0,0),(0,0)), server logscould not send pixel data: CLIENT ALREADY DISCONNECTED) to the correct test pattern — after the DesktopSize rect the client now re-requests and receives the RAW frame before completing:Supersedes #342 (the test-only repro PR): this PR carries that test in final form, renamed for the behaviour it checks per the repo convention. When this merges, close #342 unmerged — its
expectedFailuretest would report an unexpected success on a fixed tree.